-
Notifications
You must be signed in to change notification settings - Fork 264
Socks5 in GW probe #6298
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Socks5 in GW probe #6298
Conversation
|
Thank you for making this first PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
4b118a7 to
05448ab
Compare
05448ab to
e4948e6
Compare
| [package] | ||
| name = "nym-node-status-agent" | ||
| version = "1.0.7" | ||
| version = "1.0.8-gw-socks5" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does it make sense to annotate the version with gw-socks5 suffix? it will either have it or not. i.e. there wouldn't be 1.0.8 that does not have it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this won't stay, I needed a version which wouldn't conflict with what we'll use in prod to build & deploy in a test env
|
|
||
| /// endpoint to test against | ||
| /// https://www.quicknode.com/docs/ethereum/web3_clientVersion | ||
| const TARGET_URL: &str = "https://docs-demo.quiknode.pro"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are we testing against this quite random website? why not something we control or something that's more used for this purpose
| ) -> anyhow::Result<ProbeResult> { | ||
| let exit_gateway = match gateway_key { | ||
| Some(gateway_key) => NodeIdentity::from_base58_string(gateway_key)?, | ||
| None => directory.random_exit_with_nr()?, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the point of testing a random gateway for socks5?
| if let Some(ref nr_details) = node_info.network_requester_details { | ||
| match do_socks5_connectivity_test( | ||
| &nr_details.address, | ||
| NymNetworkDetails::new_from_env(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if env is not set?
| }; | ||
|
|
||
| info!("Waiting for network topology to be ready..."); | ||
| tokio::time::sleep(Duration::from_secs(10)).await; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
couldn't we wait for some condition rather than always wait 10s? what if topology takes longer than that? (or way less than that)?
This change is